Use hiredis command packing for async connections - #4191
Conversation
petyaslavova
left a comment
There was a problem hiding this comment.
Hey @sakshichitnis27, thanks for picking this up — closing the async hiredis-packing gap from #4176 is worth doing, and the layering is right. One blocker before we can merge, plus a couple of follow-ups.
Blocker: the hiredis path ignores the connection encoder, so it always encodes str as UTF-8. Async currently honors a custom encoding (e.g. encoding="latin-1" packs café as caf\xe9); with this change and hiredis installed it becomes caf\xc3\xa9. That silently changes the bytes written to Redis for anyone using a non-UTF-8 encoding, so it isn't a no-op / "no public API changed" as the description states. Please preserve the existing behavior — either fall back to the Python packer when encoding isn't the default, or pre-encode the args through the connection encoder before calling hiredis.pack_command — and add a regression test that covers a non-UTF-8 encoding, under both protocols.
Two follow-ups: (1) the new async HiredisRespSerializer diverges from the sync one (fallback-based, different memoryview handling) — let's settle the shared-vs-separate question you raised in the issue and, ideally, share one implementation; (2) the current tests monkeypatch hiredis.pack_command, so they don't verify real wire output — an async equivalent of tests/test_connection.py::test_pack_command would lock it down. Note the sync HiredisRespSerializer has the same encoder gap, so the encoding fix likely belongs in both stacks.
|
@petyaslavova |
Fixes #4176
Description of change
Async connections currently always use the Python command-packing implementation, even when
hiredis.pack_commandis available.This change uses hiredis command packing for async connections when hiredis is installed, while retaining the existing Python fallback. Commands containing memoryviews continue to use the Python path to preserve the existing behavior.
Focused tests cover both hiredis selection and the no-hiredis fallback.
Testing
python -m pytest tests/test_asyncio/test_connection.py -qpython -m pytest tests/test_asyncio/test_encoding.py -qinvoke lintersPull Request check-list
Note
Low Risk
Localized to command serialization with explicit memoryview fallback and new tests; no auth or connection lifecycle changes.
Overview
Async connections now use hiredis for RESP command packing when the extension is installed, matching sync behavior instead of always going through the pure-Python packer.
pack_commanddelegates to a sharedHiredisRespSerializerwired with the async Python fallback andencoder.encode. Commands that include memoryview arguments still use the Python path. The syncHiredisRespSerializeris refactored to take injectable fallback/encode callables so encoding (e.g.latin-1) is applied consistently on the hiredis path.Tests cover hiredis vs no-hiredis selection and connection encoding for sync and async.
Reviewed by Cursor Bugbot for commit 95eff2f. Bugbot is set up for automated code reviews on this repo. Configure here.